Errors reported only from the first failing linter

[jaen]

Describe the bug

I decided to try using go-based treefmt to see if it doesn't have issues with formatting hidden files and I noticed that while it now works with hidden files, it seems to report failures for only the first formatter that fails.

To Reproduce

Steps to reproduce the behaviour:

1. use this contrived treefmt.toml (this was generated by treefmt-nix, so you'd have to adjust paths for your system):

   [formatter]
   [formatter.actionlint]
   command =  "/nix/store/r9h133c9m8f6jnlsqzwf89zg9w0w78s8-bash-5.2-p15/bin/bash"
   excludes = []
   includes = [".github/workflows/*.yml", "./github/workflows/*.yml"]
   options = ["-eucx", "sleep 2; /nix/store/gksvgsiwzwc69lm7rm54wgfll26c2dyp-actionlint-1.6.26/bin/actionlint"]
   
   [formatter.ruff-check]
   command = "/nix/store/r9h133c9m8f6jnlsqzwf89zg9w0w78s8-bash-5.2-p15/bin/bash"
   excludes = ["apks/chromium/*"]
   includes = ["*.py", "*.pyi"]
   options = ["-eucx", "sleep 1; /nix/store/372fyydc1xp2mjj1wwd8n28qlp0fp5dg-ruff-0.1.6/bin/ruff check ."]

2. run treefmt and observe you see errors from ruff only,
3. switch the sleep durations around,
4. run treefmt and observe you see errors from actionlint only.

Expected behaviour

Errors from all failing formatters/linters are reported.

System information

I used nixpkgs-unstable#treefmt2 which at this point in time points to 2.0.0-rc4.

[jaen]

After some more experimentation I've discovered those two things:

• when one of the formatters is shfmt it seems to leave some intermediary files (e.g. .update.sh2364291851 for update.sh) non-deterministically. This might mean that other formatters have their processes killed too early when another fails (or might be a separate issue),

• after some further experimentation, it seems that is in fact the case – only the first formatter to finish using the contrived config below outputs it's .done.formatter file, the other are killed as soon as sleep is finished (because it's blocking, I guess):

  [formatter]
  [formatter.actionlint]
  command =  "/nix/store/r9h133c9m8f6jnlsqzwf89zg9w0w78s8-bash-5.2-p15/bin/bash"
  excludes = []
  includes = [".github/workflows/*.yml", "./github/workflows/*.yml"]
  options = ["-eucx", "shopt -s globstar; sleep 1 && /nix/store/gksvgsiwzwc69lm7rm54wgfll26c2dyp-actionlint-1.6.26/bin/actionlint .github/workflows/*.yml || export exitcode=$? && date +%s > .done.actionlint && echo exitcode=${exitcode:-0}  >> .done.actionlint && exit ${exitcode:-0}"]
  
  [formatter.ruff-check]
  command = "/nix/store/r9h133c9m8f6jnlsqzwf89zg9w0w78s8-bash-5.2-p15/bin/bash"
  excludes = ["apks/chromium/*"]
  includes = ["*.py", "*.pyi"]
  options = ["-eucx", "shopt -s globstar; sleep 2 && /nix/store/372fyydc1xp2mjj1wwd8n28qlp0fp5dg-ruff-0.1.6/bin/ruff check **/*.py || export exitcode=$? && date +%s > .done.ruff-check && echo exitcode=${exitcode:-0} >> .done.ruff-check && exit ${exitcode:-0}"]
  
  [formatter.shfmt]
  command = "/nix/store/r9h133c9m8f6jnlsqzwf89zg9w0w78s8-bash-5.2-p15/bin/bash"
  excludes = []
  includes = ["*.sh"]
  options = ["-eucx", "shopt -s globstar; sleep 3 && /nix/store/29ch5nlxyj31v36syqsl9ylgn6h6p9yb-shfmt-3.7.0/bin/shfmt -i 2 -s -w **/*.sh || export exitcode=$? && date +%s > .done.shfmt && echo exitcode=${exitcode:-0} >> .done.shfmt && exit ${exitcode:-0}"]
  
  [formatter.nixpkgs-fmt]
  command = "/nix/store/r9h133c9m8f6jnlsqzwf89zg9w0w78s8-bash-5.2-p15/bin/bash"
  excludes = []
  includes = ["*.nix"]
  options = ["-eucx", "shopt -s globstar; sleep 10 && /nix/store/7714qjhzws6rcq5227w1859g3kl5ma62-nixfmt-unstable-2024-05-28/bin/nixfmt . || export exitcode=$? && date +%s > .done.nixpkgs-fmt && echo exitcode=${exitcode:-0} >> .done.nixpkgs-fmt && exit ${exitcode:-0}"]

[brianmcgee]

The errgroup will cancel any outstanding tasks on first error. As you've described, it's currently designed to abort when the first formatter fails. At the very least this should be more graceful, allowing any formatters in flight to complete rather than killing them. I think I'm passing the context into exec, perhaps I shouldn't be.

I never considered letting all the formatting run it's course, reporting errors as they occur on a best-effort basis. I'd like to think through the implications of that a bit more before making a change like this.

@zimbatm I'd like to get your input too.

[jaen]

FWIW as far as I can tell, the Rust-based treefmt runs all formatters/linters to completion and returns all errors — at least in the actual setup I get failures for both mypy and ruff-check in a single run and all the other files get formatted as well.

So at the very least keeping this behaviour will be a breaking workflow change to users of the current treefmt version. I don't particularly care as I've only started trying treefmt out (though I think the existing behaviour is better, personally), but existing users might.

[brianmcgee]

To be honest that settles it then.

One of the primary aims with 2.0 is to avoid breaking anything for existing users. We can diverge in behaviour and feature set in 2.{1-} onwards. So if treefmt runs everything to completion and spits out all errors, then the go version needs to as well.

[jaen]

Seems to work better now, thanks!

[jaen]

@brianmcgee ok, looks like it's not quite perfect – if you take the code from the PR linked above, reset to the first commit (the one adding treefmt config only) and run treefmt --no-cache then Python files won't get formatted with Ruff. But when I comment out the Ruff lint configuration, they will suddenly get formatted (I assume it might be due to sharing the formatter exectutable, but didn't test that assumption yet).

So it seems there's still some cancelling going on. Not sure if you want to re-open this, treat it as a separate issue or need a simpler reproducer.

[brianmcgee]

@jaen re-opened this issue so we retain all the context.

@zimbatm can you have a look?