Repository navigation
fix Unused error code(s) in # pyrefly: ignore: unused-ignore #3367 - #3797
asukaminato0721 wants to merge 1 commit into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Summary: - Support `# pyrefly: ignore-errors[code]` as a file-level suppression for selected Pyrefly error codes. - Reuse the existing `Suppression` representation for file-level directives so whole-file and line-level Pyrefly code matching share the same semantics, including parent suppression names. - Keep non-Pyrefly `ignore-errors` directives as blanket-only and document the new Pyrefly form. Fixes #3723 Import note (maintainer changes on top of the PR, addressing Devmate review nits): - Dedupe the `[code, code]` parse into a shared `parse_error_codes` helper used by both the line-level and file-level parsers. - Reject malformed file-level directives with a missing closing `]` (previously silently accepted as valid). - Drop empty entries so `# pyrefly: ignore-errors[]` (and trailing commas) act as a blanket ignore rather than a directive matching nothing. - Update the `parse_ignore_all` docstring to reflect its `Vec<Suppression>` return shape. ## Notes This is scoped to file-level code filtering for `pyrefly: ignore-errors`. Related suppression lint/reporting PRs (#3832 and #3797) address separate diagnostics and unused-ignore behavior. Pull Request resolved: #3944 Test Plan: buck test pyrefly:pyrefly_library -- test_pyrefly_top_level_ignore (6 pass); buck test pyrefly_python -- test_parse_ignore (4 pass, incl. empty-bracket + missing-bracket cases). Reviewed By: kinto0 Differential Revision: D110063592 Pulled By: NathanTempest fbshipit-source-id: 734d71d546e8d39f696b31af3eaeb0a53d6f094f
Summary: - Support `# pyrefly: ignore-errors[code]` as a file-level suppression for selected Pyrefly error codes. - Reuse the existing `Suppression` representation for file-level directives so whole-file and line-level Pyrefly code matching share the same semantics, including parent suppression names. - Keep non-Pyrefly `ignore-errors` directives as blanket-only and document the new Pyrefly form. Fixes meta#3723 Import note (maintainer changes on top of the PR, addressing Devmate review nits): - Dedupe the `[code, code]` parse into a shared `parse_error_codes` helper used by both the line-level and file-level parsers. - Reject malformed file-level directives with a missing closing `]` (previously silently accepted as valid). - Drop empty entries so `# pyrefly: ignore-errors[]` (and trailing commas) act as a blanket ignore rather than a directive matching nothing. - Update the `parse_ignore_all` docstring to reflect its `Vec<Suppression>` return shape. ## Notes This is scoped to file-level code filtering for `pyrefly: ignore-errors`. Related suppression lint/reporting PRs (meta#3832 and meta#3797) address separate diagnostics and unused-ignore behavior. Pull Request resolved: meta#3944 Test Plan: buck test pyrefly:pyrefly_library -- test_pyrefly_top_level_ignore (6 pass); buck test pyrefly_python -- test_parse_ignore (4 pass, incl. empty-bracket + missing-bracket cases). Reviewed By: kinto0 Differential Revision: D110063592 Pulled By: NathanTempest fbshipit-source-id: 734d71d546e8d39f696b31af3eaeb0a53d6f094f
|
This pull request has been imported. If you are a Meta employee, you can view this in D116160143. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9da9829 to
8ef1620
Compare
This comment has been minimized.
This comment has been minimized.
8ef1620 to
d680689
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
stroxler
left a comment
There was a problem hiding this comment.
Thanks! This needs a rebase before I can merge it; trying to auto-rebase the synced diff failed.
Based on AI review, I think this also needs some fixes... I was considering stacking some fix-forwards, because I think this feature is pretty important, but because we have to rebase anyway I'm passing them off to you:
-
A stale “used” report can silently lose its protection.
[error/suppress.rs:172–183](https://github.com/meta/pyrefly/blob/71ca65a57395d4908471fd5a20e21c64d81649a9/pyrefly/lib/error/suppress.rs#L172-L183)
Generate a Windows report where a suppression is used, change only its trailing comment text, then generate a Linux report where it is unused. The records become separate keys; the Windows record is skipped without validation, and cleanup deletes the still-required suppression. I reproduced this with actual platform-specific checks.
Fix: reject inconsistent source snapshots, including stale records marked used—not just records proposing deletion. -
Mixing diagnostics JSON and usage reports bypasses usage protection.
[commands/suppress.rs:74–84](https://github.com/meta/pyrefly/blob/71ca65a57395d4908471fd5a20e21c64d81649a9/pyrefly/lib/commands/suppress.rs#L74-L84)
Passing an unused-ignore diagnostic alongside a report saying the same suppression was used still deletes it. Diagnostic edits are appended independently, so the report cannot veto them.
Fix: reject mixed input formats, or reconcile all usage evidence before producing edits. -
--check-allproduces cleanup records for bundled, non-filesystem sources.
[state/errors.rs:700–704](https://github.com/meta/pyrefly/blob/71ca65a57395d4908471fd5a20e21c64d81649a9/pyrefly/lib/state/errors.rs#L700-L704)
My minimal check produced 127 bundled-source records with relative paths. Consuming it with--remove-unused=allfailed trying to read_collections_abc.pyi, aborting cleanup.
Fix: restrict actionable reports toModulePathDetails::FileSystem, matchingSerializedError::from_error.
Scope: the summary still promises self-suppression, but the patch deliberately retains those diagnostics and implements report aggregation instead. Update the summary to explain that change in approach.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
stroxler
left a comment
There was a problem hiding this comment.
Looks like this was rebased, but the issues above were not yet addressed.
Also, AI review caught one more potential problem:
[commands/check.rs:1955–1960](https://github.com/meta/pyrefly/blob/3c82d7bf839b872f840a30128fa2005382fefba7/pyrefly/lib/commands/check.rs#L1955-L1960): the suppression report uses BufWriter without explicitly flushing it. For small reports, writing happens during drop, where write errors are discarded.
The AI reviewer reproduced this on the updated version: checking an error-free file with --report-suppressions /dev/full --output-format=omit-errors --summary=none returns exit code 0 with no error, although the report cannot be written.
Fix: call writer.flush()? after writing the newline, and add a regression test for write failure.
Summary
Fixes #3367
# pyrefly: ignore[unused-ignore]now suppresses the display/reporting of the unused-ignore diagnostic for its own ignore comment, including mixed-code directives like # pyrefly: ignore[bad-return, unused-ignore].The raw unused-ignore collector still returns those errors, so --remove-unused-ignore behavior remains unchanged.
Test Plan
add test