Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[red-knot] Handle pipe-errors gracefully #16354

Merged
merged 1 commit into from
Feb 25, 2025
Merged

[red-knot] Handle pipe-errors gracefully #16354

merged 1 commit into from
Feb 25, 2025

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Feb 24, 2025 •
edited
Loading

Summary

This PR removes the use of println from the Red Knot CLI in favor of using writeln which isn't prone to panicking when writing to a broken pipe.

This PR also adds some code to exit without an error code if the root cause for erroring is a broken pipe (copied over from Ruff)

Fixes #15586

Test Plan

@carljm tested it

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Feb 24, 2025
carljm
carljm approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this!

BurntSushi
BurntSushi approved these changes Feb 24, 2025
View reviewed changes
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

LGTM!

@carljm
Copy link
Contributor

carljm commented Feb 24, 2025

I haven't found a good way to trigger the broken pipe error in Red Knot. So this is mostly untested

I thought @JelleZijlstra mentioned in Discord that this is triggerable by piping the output from red-knot to less and then Ctrl-C? Does that not fail for you?

@BurntSushi
Copy link
Member

That or head. You usually need "enough" output to trigger it (because it needs to overflow a buffer somewhere).

@carljm
Copy link
Contributor

carljm commented Feb 24, 2025

Ah that makes sense, yeah I think Jelle ran into it running red-knot on a bigger project that would have triggered lots of diagnostic output.

@MichaReiser
Copy link
Member Author

Let me try head tomorrow.

I thought @JelleZijlstra mentioned in Discord that this is triggerable by piping the output from red-knot to less and then Ctrl-C? Does that not fail for you?

That's why I wanted to get this fixed :)

@carljm
Copy link
Contributor

carljm commented Feb 24, 2025

Yeah I can trigger the panic on main by running red-knot on Black and piping it to head. With this PR, same scenario doesn't panic. 🎉

@JelleZijlstra
Copy link
Contributor

I thought @JelleZijlstra mentioned in Discord that this is triggerable by piping the output from red-knot to less and then Ctrl-C?

I think I just did "q" to quit less, but that's effectively the same.

@MichaReiser
Copy link
Member Author

I think I just did "q" to quit less, but that's effectively the same.

I tried that but i was never successful at hitting Ctrl+C right when red knot was about to print a diagnostic.

@carljm
Copy link
Contributor

carljm commented Feb 24, 2025

I tried that but i was never successful at hitting Ctrl+C right when red knot was about to print a diagnostic.

From my testing on macOS, no particular timing is necessary, just a large amount of output. ./target/debug/red_knot check --project ../black --target-version 3.12 | less followed by a q (any amount of time later) reproduces it; the key factor is just that running red-knot on black generates lots of diagnostics.

It may be different on Linux? Output buffer sizes may differ.

@MichaReiser MichaReiser merged commit d895ee0 into main Feb 25, 2025
21 checks passed
@MichaReiser MichaReiser deleted the micha/pipe-errors branch February 25, 2025 07:42
dcreager added a commit that referenced this pull request Feb 25, 2025
@dcreager
Merge branch 'main' into dcreager/alist-for-live-bds
fb2f945 
* main: (38 commits)
  [red-knot] Use arena-allocated association lists for narrowing constraints (#16306)
  [red-knot] Rewrite `Type::try_iterate()` to improve type inference and diagnostic messages (#16321)
  Add issue templates (#16213)
  Normalize inconsistent markdown headings in docstrings (#16364)
  [red-knot] Better diagnostics for method calls (#16362)
  [red-knot] Add argfile and windows glob path support (#16353)
  [red-knot] Handle pipe-errors gracefully (#16354)
  Rename `venv-path` to `python` (#16347)
  [red-knot] Fixup some formatting in `infer.rs` (#16348)
  [red-knot] Restrict visibility of more things in `class.rs` (#16346)
  [red-knot] Add diagnostic for class-object access to pure instance variables (#16036)
  Add `per-file-target-version` option (#16257)
  [PLW1507] Mark fix unsafe (#16343)
  [red-knot] Add a test to ensure that `KnownClass::try_from_file_and_name()` is kept up to date (#16326)
  Extract class and instance types (#16337)
  Re-order changelog entries for 0.9.7 (#16344)
  [red-knot] Add support for `@classmethod`s (#16305)
  Update Salsa (#16338)
  Update Salsa part 1 (#16340)
  Upgrade Rust toolchain to 1.85.0 (#16339)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carljm carljm carljm approved these changes

@BurntSushi BurntSushi BurntSushi approved these changes

@AlexWaygood AlexWaygood Awaiting requested review from AlexWaygood AlexWaygood is a code owner

@sharkdp sharkdp Awaiting requested review from sharkdp sharkdp is a code owner

Assignees
No one assigned
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Panic broken pipe when trying to calculate number of results in red_knot
4 participants
@MichaReiser @carljm @BurntSushi @JelleZijlstra