Broken "Fix All" VSCode code action for Notebooks

[HenryDashwood]

To reproduce:

Python version: 3.11.1
Ruff version: 0.1.5
Ruff-vscode version: v2023.46.0

VScode settings.json:

{
  "editor.rulers": [119],
  "editor.defaultFormatter": "esbenp.prettier-vscode",
  "editor.formatOnSave": true,
  "[python]": {
    "editor.formatOnSave": true,
    "editor.codeActionsOnSave": {
      "source.fixAll": true,
      "source.organizeImports": true
    },
    "editor.defaultFormatter": "charliermarsh.ruff"
  },
  "notebook.codeActionsOnSave": {
    "source.fixAll": true,
    "source.organizeImports": true
  },
  "explorer.confirmDragAndDrop": false,
  "github.copilot.enable": {
    "*": true,
    "plaintext": false,
    "markdown": true,
    "scminput": false
  },
  "jupyter.askForKernelRestart": false,
  "jupyter.widgetScriptSources": ["jsdelivr.com", "unpkg.com"],
  "[sql]": {
    "editor.defaultFormatter": "inferrinizzard.prettier-sql-vscode"
  },
  "editor.minimap.enabled": false,
  "Prettier-SQL.keywordCase": "upper"
}

Any notebook will do. E.g. in cell 1:

from pathlib import Path

In cell 2:

Path.cwd()

Saving this notebook will cause from pathlib import Path to be deleted from the first cell. Commenting out "source.fixAll": true, in the notebook.codeActionsOnSave bit of settings.json stops this but obviously also turns off Ruff's functionality.

It's possible I've done something wrong or this is expected behaviour but just flagging it in case it's useful. Really amazing how much you've been shipping lately!

[HenryDashwood]

In case anyone else sees this the following configuration works

{
  "editor.rulers": [119],
  "editor.defaultFormatter": "esbenp.prettier-vscode",
  "editor.formatOnSave": true,
  "notebook.formatOnSave.enabled": true,
  "[python]": {
    "editor.formatOnSave": true,
    "editor.codeActionsOnSave": {
      "source.fixAll": true,
      "source.organizeImports": true
    },
    "editor.defaultFormatter": "charliermarsh.ruff"
  },
  "notebook.codeActionsOnSave": {
    "source.organizeImports": true
  },
  "explorer.confirmDragAndDrop": false,
  "github.copilot.enable": {
    "*": true,
    "plaintext": false,
    "markdown": true,
    "scminput": false
  },
  "jupyter.askForKernelRestart": false,
  "jupyter.widgetScriptSources": ["jsdelivr.com", "unpkg.com"],
  "[sql]": {
    "editor.defaultFormatter": "inferrinizzard.prettier-sql-vscode"
  },
  "editor.minimap.enabled": false,
  "Prettier-SQL.keywordCase": "upper"
}

[dhruvmanila]

I think this is a problem.

[MichaReiser]

I think this is a problem.

Do you have an idea why it is happening? Is it because the LSP only sees a single cell and doesn't know about the entire document or is this also an issue when running ruff (without the LSP)?

[dhruvmanila]

Do you have an idea why it is happening?

Yes, sorry I discussed this on Discord. To give context, this is happening because the "notebook.codeActionsOnSave" sends the code action request for each cell. Now, this is a source level code action which means that our server invokes the Ruff command, collects the diagnostics and applies any necessary fixes. But, this means that it doesn't have context from other cells. So, if first cell has import math and second cell has math.pi, invoking the builtin Fix All on the first cell will remove the import.

This is only for the builtin command Fix All (not the one with Ruff: prefix). Organize imports should work because it doesn't require context from other cells. I think the right behavior for source.fixAll is to look at the available diagnostics for the current cell and fix them instead of invoking Ruff. But, the request only provides diagnostics for the cursor line.

[blakeNaccarato]

Hi @dhruvmanila, I know you already responded to my comment astral-sh/ruff-vscode#256 (comment) over in astral-sh/ruff-vscode#256, but I'll mention it again here in case it helps somehow. I see you've discussed notebook/cell tradeoffs over in #264 (comment), so I hope I'm not doubling-down on irrelevant information here, I promise I won't bring it up again 😅

The Notebook CodeActionKind was exposed to VSCode extension authors recently. If ruff-lsp was able to operate on this action, I think it could receive the entire notebook code context rather than individual cells, if I understand correctly. Basically the current settings are possible with Ruff, notice the absence of notebook prefix before source in the subkeys:

  "notebook.codeActionsOnSave": {
    "source.fixAll.ruff": "explicit",
    "source.organizeImports.ruff": "explicit",
  },

And that seems to operate on a per-cell basis. If the Notebook CodeActionKind were set up over in the VSCode extension repo for Ruff, then I think it would hand over the full notebook code contents to ruff-lsp for context. I know you mentioned that LSP layer is responsible for everything so this may be irrelevant, but maybe some handshake between LSP and VSCode extension would facilitate receiving full-notebook context, while maybe still being able to modify cells (not sure about that last part). The details get murky for my level of understanding around here. Leveraging this new code action kind would facilitate a settings.json like below, with notebook prefixes on each subkey.

Caution

These settings are 🚨non-operational🚨 and reflect a proposed API change to the Ruff VSCode extension. The VSCode Problems pane won't warn you that they're non-operational when setting it in settings.json .

Possible future settings.json if proposed API change is implemented:

{
 // This won't work as you expect, but VSCode won't warn you about using them
 "notebook.codeActionsOnSave": {
   "notebook.source.fixAll.ruff": "explicit",
   "notebook.source.organizeImports.ruff": "explicit",
 },

[juanitorduz]

Experiencing the same 🙏

[charliermarsh]

Interesting, @dhruvmanila can we use these "notebook.source.fixAll.ruff" settings?

[Karl60]

Adding the notebook prefix giving "notebook.source.fixAll.ruff" solved the import deletion problem for me.

[blakeNaccarato]

Caution

@Karl60 your fix may be a side-effect of rearranging your notebook.codeActionsOnSave, since the 🚨non-operational🚨 "notebook.source.fixAll.ruff" entry is not currently supported by the Ruff VSCode extension, even though the VSCode Problems pane won't warn you about this when setting it in settings.json .

If Ruff doesn't register that fixer, and have it hooked up to code that actually handles that case, then using that setting will have no effect. Obscuring things further, you also won't be warned by VSCode about this, as it does not validate entries in notebook.codeActionsOnSave. Any non-operational entries will silently do nothing, typos silently pass, etc.

I know it's a bit of a game of telephone here, I've edited/strengthened my disclaimer in #320 (comment) that 🚨non-operational🚨 "notebook.source.fixAll.ruff" is just a proposed way that Ruff developers may be able to remedy this problem, and it won't function unless a PR explicitly implements a notebook-wide fixer via this method.

Another clarification (not to your point, but generally), ruff-lsp and the Ruff VSCode extension does already support notebook-wide fixing through a different pathway. It's just that the 🚨non-operational🚨 "notebook.source.fixAll.ruff" pathway only just became possible to implement a couple months ago when that functionality was exposed in the extension API.

I hope that clears things up! I hope my tone isn't appearing too pedantic here, I'm just trying to be hyper-explicit to avoid the proliferation of a setting that would enable a proposed Ruff VSCode API that doesn't exist yet.

[charliermarsh]

Thanks @blakeNaccarato, it's really helpful! @dhruvmanila is the best person to implement this and he's taking some well-earned vacation. I may be able to get to it before he gets back, but otherwise it'll be tackled as soon as he can :)

[charliermarsh]

@karthiknadig - do you know if the lsprotocol package needs to be updated to support "notebook.source.fixAll" and related actions?

[charliermarsh]

Actually, maybe I'm misunderstanding, since I only see that in the VS Code API reference and not the LSP spec.

[karthiknadig]

Code actions can be custom, the "notebook" namespace is one of the custom ones. This should work with the current lsprotocol stable.

[charliermarsh]

Okay, I believe I know how to fix this. We need to register handlers for the notebook.* versions, then use the full document when those are triggered (even though they only pass in the URI of the first cell).

[charliermarsh]

This fixes the on-save actions but unfortunately running "Fix all" from the command palette will still have the same problem, since that runs source.fixAll, which only looks at the current cell.

[karthiknadig]

@Yoyokrazy might have more details on the command pallet for notebook fix all.

[blakeNaccarato]

Preface: I'm putting this here since it's still a fresh fix and all the relevant parties are already subscribed, but if you want this raised in a separate issue I'll do that as well.

What a quick turnaround! I took this for a hacky test run, and notebook.source.fixAll works. Awesome!

However, it looks like notebook.source.organizeImports seems to still be a no-op, at least on my machine/setup. Changing that one back to source.organizeImports brings import organization back.

In other words, the following settings.json key fixes errors and organizes imports without clobbering (notebook prefix omitted from the second entry):

 "notebook.codeActionsOnSave": {
   "notebook.source.fixAll": "explicit",
   "source.organizeImports": "explicit"
 },

While this only fixes errors, but does not organize imports:

Caution

 "notebook.codeActionsOnSave": {
   "notebook.source.fixAll": "explicit",
   "notebook.source.organizeImports": "explicit"
 },

However, if the former setup works, and the latter doesn't, then maybe it's just the case that guidance should be for fixAll to be run in notebook mode, and organizeImports in the old way, at least in the interim. This forces import sorting to come after fixes (see context below, same one mentioned in the PR), so they're not getting clobbered like they tended to do before this latest implementation.

microsoft/vscode#193120

• If a notebook CodeAction is called against a notebook, it will ONLY be called against the first cell of the notebook, and will precede the standard source CodeActions

I think this means source.organizeImports always operates after notebook.source.fixAll, which might be why it works as just described. This suggests that notebook.source.organizeImports operates prior to (or concurrently with?) notebook.source.fixAll, and fixAll is winning the race or something. Your test plan in #351 doesn't explicitly exercise notebook.source.organizeImports, but maybe your test suite is doing that anyways.

There are a few situations where users have resorted to passing a list of options to the ...codeActionsOnSave setting, which is probably not a good idea here, but some of the discussion about the mechanics of action execution order may help iron out this wrinkle (microsoft/vscode#88131, microsoft/vscode#194861).

Here's more detail in my settings.json for this test, with some comments as to settings I fiddled with during.

relevant `settings.json` snippet from that repo

{
  //* ruff
  "ruff.importStrategy": "fromEnvironment",  // Tried with and without this set
  "ruff.format.args": [],  // These explicitly clear out possible user settings overrides
  "ruff.lint.args": [],
  "[python]": {
    "editor.defaultFormatter": "charliermarsh.ruff"
  },
  "[ipynb]": {
    "editor.defaultFormatter": "charliermarsh.ruff"
  },
  //* Formatting
  "editor.formatOnPaste": true,
  "editor.formatOnSave": true,
  "editor.formatOnSaveMode": "file",
  "editor.formatOnType": false,
  "editor.codeActionsOnSave": {
    "source.fixAll": "always",
    "source.organizeImports": "always"
  },
  //* Notebook
  //? https://github.com/microsoft/vscode/issues/195223#issuecomment-1800137313
  "notebook.insertFinalNewline": false,
  //? Code actions on notebook save partially work now
  //? Need to raise an issue about `notebook.source.organizeImports` not behaving
  //? https://github.com/astral-sh/ruff-lsp/issues/320
  "notebook.formatOnCellExecution": true,
  "notebook.formatOnSave.enabled": true,
  "notebook.codeActionsOnSave": {
    "notebook.source.fixAll": "explicit",
    "notebook.source.organizeImports": "explicit"
  },
}

[charliermarsh]

Hmm, I do see the correct behavior on my end from "notebook.source.organizeImports": true. As in, when I save, I see imports sorted in all cells.

[blakeNaccarato]

That's good news. Okay I'll play with it and if I manage to nail down a minimal/reproducible situation, I'll raise a separate issue about it specifically whenever I have some actionable information. If I manage to fix my setup in the process, then it's a win-win.

Thanks for the release, it definitely helps to get the code out there in the sunlight! The on save behavior is already more stable now even with my half-measure configuration.

[charliermarsh]

Definitely, and thanks for all your help! If it's not working for you, there's probably still more to investigate... (Did you try using true instead of "explicit"?)

[blakeNaccarato]

Actually yes, I did try true as well, but I didn't start from scratch or with fewer extensions/settings/global settings.

It seems that tool interactions (between e.g. Pylance, Sourcery, Ruff), especially when it comes to code actions, contribute to most of the uncertainty across dev environments.

I'll probably start from a no frills Ruff dev environment and gradually incorporate my tools until something breaks.

In any case, I won't ping this issue again about it. Just joined the Ruff Discord, so I'll chat there with any half-baked ideas 😅.