`fixit fix` exits with `0` even if there were errors

[samueljsb]

fixit fix will exit with an exit code of 0 if it finds errors but does not apply fixes. This makes using it in CI (or pre-commit hooks) awkward because it hides errors that can't be auto-fixed. The solution is to run the tool *twice: once to fix fixable errors, then once again to re-report them in a way that can cause a Ci job to fail. This is a frustrating duplication of work.

Given the outcome of #258, I think this is a bug rather than intentional design, but I'm happy to be corrected if there's another change I've missed 🙂

There are two cases where I would expect a non-zero exit code from fixit fix.

failure to run

When it fails to evaluate the rules at all because of an error:

$ cat t.py
this is not valid python

$ fixit fix -a t.py
t.py: EXCEPTION: Syntax Error @ 1:1.
parser error: error at 1:24: expected one of !=, %, &, (, *, **, +, ,, -, ., /, //, ;, <, <<, <=, ==, >, >=, >>, @, NEWLINE, [, ^, and, if, in, is, not, or, |

this is not valid python
^
Traceback (most recent call last):
  File "/private/tmp/tmpvenv-c329d/venv/lib/python3.11/site-packages/fixit/api.py", line 100, in fixit_bytes
    runner = LintRunner(path, content)
             ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/private/tmp/tmpvenv-c329d/venv/lib/python3.11/site-packages/fixit/engine.py", line 55, in __init__
    self.module: Module = parse_module(source)
                          ^^^^^^^^^^^^^^^^^^^^
  File "/private/tmp/tmpvenv-c329d/venv/lib/python3.11/site-packages/libcst/_parser/entrypoints.py", line 109, in parse_module
    result = _parse(
             ^^^^^^^
  File "/private/tmp/tmpvenv-c329d/venv/lib/python3.11/site-packages/libcst/_parser/entrypoints.py", line 55, in _parse
    return parse(source_str)
           ^^^^^^^^^^^^^^^^^
libcst._exceptions.ParserSyntaxError: Syntax Error @ 1:1.
parser error: error at 1:24: expected one of !=, %, &, (, *, **, +, ,, -, ., /, //, ;, <, <<, <=, ==, >, >=, >>, @, NEWLINE, [, ^, and, if, in, is, not, or, |

this is not valid python
^
🛠️  1 file checked, 1 file with errors 🛠️

$ echo $?
0

I would expect fixit to exit with a non-zero code in the above case.

errors found but not fixed

fixit fix also exits 0 if errors were found but no fixes were applied:

$ cat t.py
try:
    ...
except ValueError or KeyError:
    ...

$ fixit fix -a t.py
t.py@1:0 AvoidOrInExcept: Avoid using 'or' in an except block. For example:'except ValueError or TypeError' only catches 'ValueError'. Instead, use parentheses, 'except (ValueError, TypeError)'
🛠️  1 file checked, 1 file with errors 🛠️

$ echo $?
0

I would also expect it to exit non-zero in this case, although the suggestion in #258 for that to be opt-in seems reasonable if backwards-compatibility is a concern.

---

Fixit version 2.1.0 Python 3.11.5 (main, Aug 24 2023, 15:09:45) [Clang 14.0.3 (clang-1403.0.22.14.1)]

[amyreese]

The first case seems like a clear bug — we should probably match the error code that fixit lint gives in cases of these errors.

I'm interested in the CI use case for fixes here. Is CI expecting to auto-amend the fixes in this case, but still fail if the repo is not "lint clean" after fixes? The suggested CI isage is more along the lines of fixit lint --diff, while fixit fix would be used interactively or in cases where outstanding lints wouldn't be reason for the auto-fixes to produce an error code (ie, the fixing was successful, even if the code has outstanding/unfixable errors).

Would a "strict" mode (either a --strict flag or config value) to enforce lint clean results from all commands be a reasonable option?

[samueljsb]

I'm interested in the CI use case for fixes here. Is CI expecting to auto-amend the fixes in this case, but still fail if the repo is not "lint clean" after fixes?

There are two cases I'm thinking about here:

1. In CI it's useful to use the fix command with --diff because the CI job failure then shows exactly what needs to be changed to fix the problem.

2. With pre-commit, we want the fixes to be applied when they are available, so they can be committed. But we also need to fail if there are unfixable errors. The only way we have to work around this at the moment is to run fixit twice:

   -   repo: https://github.com/Instagram/Fixit
     rev: v2.1.0
     hooks:
     -   id: fixit-fix  # fixes if available, but passes if there are unfixable errors
     -   id: fixit-lint  # fails if unfixable errors

Both of these have workarounds (use lint instead of fix in CI and forego the diff; run twice with pre-commit), but it took us a long time using fixit to figure out that our linting process was faulty. It is very surprising that a result that says "there are errors" is counted as a success.