zed icon indicating copy to clipboard operation
zed copied to clipboard

Formatting issue with multiple formatters

Open failable opened this issue 1 year ago • 8 comments
trafficstars

Check for existing issues

  • [X] Completed

Describe the bug / provide steps to reproduce it

  • Create test.py and other.py (See below).
  • Set the Python formatter config to below.
  • Trigger the formatting command.
  • Observe the unexpected formatted result.

I'm not 100% sure if it's an issue of Zed (because the ruff server is handling this instead of ruff format), but simply running ruff check --fix --select I test.py and ruff format test.py in the terminal seems fine.

test.py

# Some comment here

import os

from other import fn


import numpy as np




def main():
    print(os.path.dirname(__file__))
    print(np.arange(10))
    fn()


if __name__ == "__main__":
    main()

other.py

def fn():
    pass

Python config

{
  "Python": {
    "format_on_save": "on",
    "formatter": [
      {
        "external": {
          "command": "ruff",
          "arguments": [
            "check",
            "--fix",
            "--select",
            "I",
            "-"
          ]
        }
      },
      {
        "language_server": {
          "name": "ruff"
        }
      }
    ]
  }
}

Unexpected result

image

Environment

Zed: v0.146.3 (Zed Preview) OS: macOS 14.5.0 Memory: 32 GiB Architecture: x86_64

If applicable, add mockups / screenshots to help explain present your vision of the feature

No response

If applicable, attach your Zed.log file to this issue.

Zed.log

failable avatar Jul 31 '24 16:07 failable

A few minutes after this was posted Zed Preview v0.147.x went out with multiple ruff improvements. Notably

Screenshot 2024-08-01 at 17 06 44 Screenshot 2024-08-01 at 17 07 11

Can you update, alter your config appropriately and let me know whether you're still seeing this issue? Thanks

notpeter avatar Aug 01 '24 21:08 notpeter

@notpeter Hello!

I tried this settings

"languages": {
    "Python": {
      "format_on_save": { "language_server": { "name": "ruff" } },
      "formatter": { "language_server": { "name": "ruff" } },
      "language_servers": ["pyright", "ruff"]
    }
}

in

Zed: v0.147.1 (Zed Preview)
OS: macOS 14.5.0
Memory: 32 GiB
Architecture: x86_64

the code is formatted but the imports are not sorted.

As stated in here,

Currently, the Ruff formatter does not sort imports. In order to both sort imports and format, call the Ruff linter and then the formatter: ruff check --select I --fix ruff format

and

Allow user to configure multiple formatters for a given language: format and format_on_save now accept an array of formatting actions to run;

I assumed the settings in my original post should work. But it still does not work in the latest version of Zed preview.

failable avatar Aug 02 '24 02:08 failable

@failable to sort imports, add code actions to formatters:

  "languages": {
    "Python": {
      "formatter": [
        {
          "code_actions": {
            "source.organizeImports.ruff": true,
            "source.fixAll.ruff": true
          }
        },
        { "language_server": { "name": "ruff" } }
      ],
      "format_on_save": "on",
      "language_servers": ["pyright", "ruff"]
    },

bersace avatar Aug 13 '24 09:08 bersace

@bersace Thanks. I also noticed there is a recently added setup guide in the ruff documentation last night. In this ruff specific case it works. But the multiple formatter setting (using multiple commands formatters) is still buggy.

failable avatar Aug 13 '24 09:08 failable

Yep, 0.148 seems buggy regarding formatting.

bersace avatar Aug 13 '24 09:08 bersace

The following setting (The reason why I tried this setting)

"languages": {
    "Python": {
      "language_servers": ["basedpyright", "ruff", "!pyright"],
      "format_on_save": "on",
      "formatter": [
        {
          "language_server": { "name": "ruff" }
        },
        {
          "code_actions": {
            "source.organizeImports.ruff": true,
            "source.fixAll.ruff": true
          }
        },
        {
          "language_server": { "name": "ruff" }
        }
      ]
    }
  }

will format

print(1, 2,)

into

    print(
        1,
        2,
        1,
        2,
    )

and format

    print(
        1, 2
    )

into

    print(1, 2)
        1,
        2,

It seems that the formatter is not simply executing one step after another.

failable avatar Aug 19 '24 08:08 failable

I appear to be having a related issue with the Dart formatter. I have written about it in detail in https://github.com/zed-industries/zed/issues/5072#issuecomment-2299437797, but the gist is that I must be able to run the Dart formatter first, then the spaces-to-tabs converter. It must be in that order, otherwise Dart's formatter will override the changes, ultimately rendering the code spaces-only. However, the new feature seams to make the formatters function out of order, acting quite similarly to what's happening to @failable. Please see the linked issue comment for more details.

srfsh avatar Aug 21 '24 04:08 srfsh

The support for multiple formatters is simply broken. Look at project/src/project.rs:

                            FormatOnSave::List(formatters) => {
                                for formatter in formatters.as_ref() {
                                    let diff = Self::perform_format(
                                        &formatter,
                                        server_and_buffer,
                                        project.clone(),
                                        buffer,
                                        buffer_abs_path,
                                        &settings,
                                        &adapters_and_servers,
                                        push_to_history,
                                        &mut project_transaction,
                                        &mut cx,
                                    )
                                    .await
                                    .log_err()
                                    .flatten();
                                    if let Some(op) = diff {
                                        format_operations.push(op);
                                    }
                                }
                            }

This loops over all the formatters, and for each it runs it against the current buffer contents and then diffs the output against the current contents (Project::perform_format -> Project::format_via_external_command -> buffer.update(cx, |buffer, cx| buffer.diff(stdout, cx))).

Notably, it does not run the successive formatters against the output of the last formatter. So what ends up happening is it generates a list of diffs, which may conflict, and in the case of conflicts we get broken buffer contents that don't apply changes from some formatters.

nt8r avatar Aug 24 '24 01:08 nt8r

Any chance we can have this issue fixed?

Yevgnen avatar Sep 13 '24 06:09 Yevgnen

I also ran into this issue when wanting to chain ruff format and ruff check --select I --fix. I worked around it like this now:

      "format_on_save": [
        {
          "external": {
            "command": "bash",
            "arguments": [
              "-c",
              "set -o pipefail; in=$(cat); echo \"$in\" | uv run ruff format - | uv run ruff check --select I --fix - || echo \"$in\""
            ]
          }
        }
      ]

(edit: I realized that we need to return the input string if any command in the pipe fails)

I also verified that the chaining of multiple external commands does not work as expected using this minimal setup, maybe it is useful when eventually implementing this ticket 😅

echo "sed 's/a/d/g'" > format_a.sh
echo "sed 's/b/e/g'" > format_b.sh
echo "abc" | sh format_a.sh | sh format_b.sh
# This should print: "dec"

Then configure the external commands in zed to reproduce the above chaining:

      "format_on_save": [
        {
          "external": {
            "command": "sh",
            "arguments": ["format_a.sh"]
          }
        },
        {
          "external": {
            "command": "sh",
            "arguments": ["format_b.sh"]
          }
        }
      ]

And run auto-formatting on a buffer with content abc and you only get dbc (only first command was applied).

thorbenk avatar Jan 23 '25 08:01 thorbenk