Skip to content

Conversation

@MichaReiser
Copy link
Member

Summary

We now capture panics during checking and emit a diagnostic with severity Fatal but the CLI
still exits with ExitStatus::Failure (1) which makes it indistinguishable from a normal: there are diagnostics.

This PR changes the implementation to return ExitStatus::Eror (which we code to 2)

Test Plan

I ran red knot on a project where it panics and printed the exit code:

✦35 ❯ echo $status
2

@MichaReiser MichaReiser added the ty Multi-file analysis & type inference label Apr 26, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 26, 2025

mypy_primer results

No ecosystem changes detected ✅

Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are panics the only source of "fatal" severity? If so, could we exit with a code other than 1 or 2? That would make panics easy to detect.

@MichaReiser
Copy link
Member Author

Are panics the only source of "fatal" severity? If so, could we exit with a code other than 1 or 2? That would make panics easy to detect.

Not strictly. But the fatal severity is reserved for internal errors (not project/user errors). What error code would you suggest? I did search for some reference on error codes and couldn't find any recommended error code other than 2.

@AlexWaygood
Copy link
Member

I don't know if this is in line with some convention or a black-specific practice, but black always exits with code 123 if there's an internal error https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#check

@sharkdp
Copy link
Contributor

sharkdp commented Apr 28, 2025

Rust programs exit with code 101 for panics. I think it would be reasonable to do the same, unless the "fatal" category also includes other sources of errors.

@MichaReiser MichaReiser changed the title [red-knot] Use 'error' exit code when there's at least one diagnostic with severity 'fatal' [red-knot] Use 101 exit code when there's at least one diagnostic with severity 'fatal' Apr 28, 2025
@MichaReiser
Copy link
Member Author

I don't think there's any good error code here. E.g. cargo uses 101 for any compilation error (e.g. because the input program is invalid).

I'll go with 101 for now.

@MichaReiser MichaReiser merged commit dbc137c into main Apr 28, 2025
34 checks passed
@MichaReiser MichaReiser deleted the micha/fatal-exit-with-error branch April 28, 2025 08:03
dcreager added a commit that referenced this pull request Apr 28, 2025
* main: (37 commits)
  [red-knot] Revert blanket `clippy::too_many_arguments` allow (#17688)
  Add config option to disable `typing_extensions` imports  (#17611)
  ruff_db: render file paths in diagnostics as relative paths if possible
  Bump mypy_primer pin (#17685)
  red_knot_python_semantic: improve `not-iterable` diagnostic
  [red-knot] Allow all callables to be assignable to @Todo-signatures (#17680)
  [`refurb`] Mark fix as safe for `readlines-in-for` (`FURB129`) (#17644)
  Collect preview lint behaviors in separate module (#17646)
  Upgrade Salsa to a more recent commit (#17678)
  [red-knot] TypedDict: No errors for introspection dunder attributes (#17677)
  [`flake8-pyi`] Ensure `Literal[None,] | Literal[None,]` is not autofixed to `None | None` (`PYI061`) (#17659)
  [red-knot] No errors for definitions of `TypedDict`s (#17674)
  Update actions/download-artifact digest to d3f86a1 (#17664)
  [red-knot] Use 101 exit code when there's at least one diagnostic with severity 'fatal' (#17640)
  [`pycodestyle`] Fix duplicated diagnostic in `E712` (#17651)
  [airflow] fix typos `AIR312` (#17673)
  [red-knot] Don't ignore hidden files by default (#17655)
  Update pre-commit hook astral-sh/ruff-pre-commit to v0.11.7 (#17670)
  Update docker/build-push-action digest to 14487ce (#17665)
  Update taiki-e/install-action digest to ab3728c (#17666)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants